-
Notifications
You must be signed in to change notification settings - Fork 223
Add support for executing GraphQL queries on non-dev stores #6738
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for executing GraphQL queries on non-dev stores #6738
Conversation
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
924deb3 to
aecb581
Compare
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success3597 tests passing in 1422 suites. Report generated by 🧪jest coverage report action from c9b495e |
aecb581 to
1a8018f
Compare
jordanverasamy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems generally reasonable :) tophatting now!
packages/app/src/cli/services/bulk-operations/bulk-operation-status.test.ts
Show resolved
Hide resolved
packages/app/src/cli/utilities/developer-platform-client/partners-client.ts
Outdated
Show resolved
Hide resolved
jordanverasamy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎩 , works!
|
My one question is whether it would've been possible to accomplish the same stuff with slightly fewer code changes, but I'm sure a reviewer who's more deeply knowledgeable about the CLI will be able to weigh in on that :) from a Bulk Ops perspective this change def works! |
1a8018f to
4cf88af
Compare
ryancbahan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we're coercing down the store type field into a boolean -- I'm curious of the reason for this? Would it be more type-safe and also stable in the long run to have something like storeType: []and pass all allowed store types instead of includeAllStores boolean?
4cf88af to
a2be03d
Compare
|
I think the boolean was added as a quick toggle between two hardcoded queries: one filtered to |
a2be03d to
c02e5d0
Compare
f861620 to
6da08fe
Compare
...rc/cli/api/graphql/business-platform-organizations/queries/fetch_dev_store_by_domain.graphql
Outdated
Show resolved
Hide resolved
6da08fe to
e99a896
Compare
e99a896 to
e197fba
Compare
48795cc to
a3b29e2
Compare
ryancbahan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code lgtm. I didn't tophat, but as an external stakeholder it all makes sense. I still have the question of whether the cli itself should do mutation validation, but I recognize that's plausibly a longer conversation.
a3b29e2 to
c9b495e
Compare



Add support for executing GraphQL queries on non-dev stores
Came from: https://app.graphite.com/github/pr/Shopify/cli/6689/Add-support-for-executing-GraphQL-queries-on-non-dev-stores
Made a new PR because this task was handed off to me and the rebasing caused major changes.
Resolves: https://github.com/orgs/shop/projects/208/views/34?pane=issue&itemId=144171469&issue=shop%7Cissues-api-foundations%7C1165
What
How to test your changes?
Try executing a query on a production store:
This should succeed.
Try executing a mutation on a production store:
This should fail with an error message indicating mutations can only be executed on Dev Stores.
Try executing a mutation on a development store:
This should succeed.